ST40p RX: move frame assembly into the transport + RFC 8331 codec dedup#1648
Merged
Conversation
This was referenced Jul 8, 2026
8c0e371 to
bb0bb88
Compare
95458ad to
bf67e4c
Compare
moleksy
reviewed
Jul 9, 2026
02eacdf to
d748dd5
Compare
DawidWesierski4
requested changes
Jul 14, 2026
DawidWesierski4
left a comment
Collaborator
There was a problem hiding this comment.
please change the scirpt
Rest is Fine but 22 minutes review so weak review for that
Frame-level assembly for ST 2110-40 was duplicated between the transport
session and the pipeline RX, with subtly different per-port sequence and
redundancy logic. The pipeline copy was the source of recurring seq_lost
and frame-drop regressions on redundant streams.
This change makes the transport the single owner of assembly and turns
the pipeline into a thin consumer:
- include/st40_api.h: add ST40_TYPE_FRAME_LEVEL surface - struct
st40_rx_frame_meta, framebuff_cnt/framebuff_size, notify_frame_ready,
st40_rx_put_framebuff. Back-compat shim keeps existing RTP_LEVEL
callers working unchanged.
- lib/src/st2110/st_rx_ancillary_session.c: implement the FRAME_LEVEL
path - frame slot pool, per-frame seq bitmap (popcount-derived
seq_lost across ports), pending/inflight handoff for redundant
delivery, parse + UDW validation, notify_frame_ready dispatch and
slot reclaim on app reject.
- lib/src/st2110/pipeline/st40_pipeline_rx.{c,h}: now a single
notify_frame_ready handler that pins the transport pool's UDW buffer
zero-copy into the pipeline framebuff and returns -EBUSY when the
pool is empty; frame status enum trimmed to FREE/READY/IN_USER to
match the transport-owned assembly model.
- tests/unit: drop the pipeline-only ST40p harness/tests (the logic
they covered now lives in the transport) and grow the session-level
ST40 harness/tests to cover frame assembly, redundancy, sequence
bitmap and slot pool exhaustion.
- doc: update design.md and stats_guide.md to reflect the new API and
counters.
Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
…de_packet,decode_packet} helpers RFC 8331 wire-format math (10-bit-packed DID/SDID/DC/UDW[]/checksum, ceil-to-byte, 4-byte align) and the header byte-swap dance are duplicated across TX build, RX parse, gstreamer, RxTxApp and test call sites. Add the primitives to st_ancillary.c / st40_api.h as a pure addition, no call sites migrated yet (follow-up commits). st40_get_udw()/st40_calc_checksum() are const-correct now (ABI unchanged). encode_packet()/decode_packet() do the full parity+checksum+advance dance in one call and reject udw_size > 255 (wire field is 8 bits) with -EINVAL. st_tx_ancillary_session.c intentionally does not adopt the encoder: its TX_ANC_TEST_APPLY_PARITY hook needs direct primitive access. New tests: rfc8331_payload_bytes_test.cpp, rfc8331_codec_test.cpp. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
rx_anc_slot_parse_pkt() delegates per-ANC-subpacket wire parsing (header decode, parity, checksum, UDW extraction) to st40_rfc8331_decode_packet() instead of hand-rolling it inline. Partial-success semantics and stat_anc_pkt_parse_err bumps are preserved; meta_entry->udw_offset is set from the frame-relative slot->udw_buffer_fill after each successful decode. Two intentional behavior deltas vs the old inline parser: DID/SDID/DC parity is now validated (previously wasn't), and the checksum is now validated even for udw_size == 0 sub-packets (previously skipped, though RFC 8331 defines the checksum over DID+SDID+DC+UDW regardless of UDW length). Both are pinned by new tests: DecodeChecksumFailZeroUdw, ChecksumFailureRecordedForEmptyAncPacket. Also adds CrossPacketUdwOffsetThreadsAcrossRtpPackets, pinning that udw_offset threads correctly across two separate RTP packets within the same frame. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
tx_ancillary_session_build_packet()/build_rtp_packet() use st40_rfc8331_payload_bytes() for the size formula and st40_rfc8331_rtp_hdr_bswap()/payload_hdr_bswap() at all 9 htonl() toggle sites in this file. Pure mechanical substitution, no behavior change (314/314 unit tests pass unchanged). TX still calls st40_set_udw()/tx_ancillary_apply_parity()/ st40_calc_checksum() directly rather than st40_rfc8331_encode_packet(): tx_ancillary_apply_parity()'s TX_ANC_TEST_APPLY_PARITY test hook needs direct primitive access, which the single-call encoder can't accommodate. Documented on encode_packet()'s doc comment so the gap doesn't need git-archaeology to explain. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
Replace manual ntohl/htonl byte-swap pairs with st40_rfc8331_payload_hdr_bswap() in gst_mtl_st40_rx.c, gst_mtl_st40p_tx.c, rx_ancillary_app.c and tx_ancillary_app.c. Also fixes two independent latent formula bugs found while migrating, both undercounting the aligned payload size by 4 bytes at udw_size % 16 == 12 (verified numerically over the full 0-255 range): gst_mtl_st40_rx_shift_payload_hdr()'s bespoke WORD_10_BIT_ALIGN formula misparses the next ANC sub-packet when triggered; RxTxApp's floor-division formula sends 4 bytes of uninitialized mbuf tail on TX and over-advances the payload pointer on RX. Both now use st40_rfc8331_payload_bytes(), same bug class as 2082f10/c0f12995. New test pins the exact divergence. The gstreamer plugin cannot be compiled/tested in this environment (gstreamer-1.0 dev package not installed); verified by code review and the numeric proof instead. RxTxApp is covered by the production build. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
…pers Replace manual ntohl/htonl byte-swap pairs and duplicated payload-size formulas in st40_test.cpp, st40i_tests.cpp and st40_harness.c with st40_rfc8331_payload_hdr_bswap()/st40_rfc8331_rtp_hdr_bswap()/ st40_rfc8331_payload_bytes(). st40_test.cpp's tx_anc_build_rtp_packet() and rx_handle_rtp() used a floor-division variant of the size formula (missing the +7 before the /8 ceil-division, and an alignment step that adds 4 extra bytes when already 4-byte aligned instead of 0). Adopting the canonical helper fixes this rather than reproducing the bug. st40i_tests.cpp already had the correct ceil formula; st40_harness.c's ut40_anc_payload_bytes() now delegates to the canonical helper instead of duplicating it. Individual translation units for the two integration test files were verified to compile cleanly (zero warnings under -Werror -Wall -Wvla); a full KahawaiTest relink could not be completed in this environment due to build/tests/ object files being repeatedly evicted between shell invocations (reproduced on unrelated, unedited files too, so this is an environment artifact, not a code issue -- see /memories/repo/build-notes.md). st40_harness.c is covered by the full unit suite: 314/314 pass. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
'From the ST40_TYPE_FRAME_LEVEL transport refactor onwards' describes what changed rather than how the stat works today. stats_guide.md documents current behavior, not history — drop the changelog phrasing, keep the factual description. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
…run_gtest Observed failure mode: an agent called run_gtest relying on port auto-discovery without checking dpdk_devbind_status first, hit a bare PF bound directly to vfio-pci (0 VFs, invalid for E810/E830), and spent many extra tool calls diagnosing multicast-join/IGMP failures before finding the real cause. Add a VF-binding check before the first run_gtest call, and a dedicated "Multicast join / IGMP failure" decision-tree entry. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
tx_ancillary_session_build_packet() and build_rtp_packet() built each ANC sub-packet by hand (did/sdid/data_count/UDW/checksum) because the BAD_PARITY test hook needed to intercept every parity-encoded field. Both now call the shared st40_rfc8331_encode_packet() codec instead; TX_ANC_TEST_APPLY_PARITY is replaced by TX_ANC_TEST_CORRUPT_PARITY, a post-encode corrective pass on the finished wire buffer. Deriving room from the accumulated payload offset exposed an unsigned underflow once the running byte count reached max_pkt_len (the prior loop-entry break only estimated UDW bit cost, not per-packet fixed overhead), which wrapped room to a huge value and defeated the new bounds check. Both loops now bail out via the existing STI_FRAME_ANC_TOO_LARGE path before the subtraction can wrap, and the break heuristic uses the exact per-packet byte size instead. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
install_dependencies, dpdk_build, ice_driver_rebuild, build_mtl, and the ecosystem/eBPF build tools returned entire build logs (hundreds to thousands of lines) inline in the tool result. Add _summarize_output(), reusing the existing _save_test_log() log-file convention from run_gtest, to save the full output to build/logs/ and return only the log path, line count, and last 40 lines. run_gtest now uses the same helper instead of its own inline tail/log-path logic. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
tx_ancillary_test_drop_pkt() called dbg() without a visible declaration when this header was pulled into a translation unit that doesn't already include mt_log.h (e.g. the ST40 TX unit-test harness), failing the debug build with -Werror=implicit-function- declaration. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
St20p/St30p/St40p unit harnesses embed the production RX pipeline .c files and relied on --allow-multiple-definition to let their fake transport functions (e.g. ut20p_rx_put_framebuff) override the real ones. Debug link ordering instead selected the real st20_rx_put_framebuff(), which dereferenced the harness's fake transport handle and crashed in mt_handle_acquire(). Each harness now #defines the transport calls to its own stub names while compiling the included pipeline source, so the harness never depends on which duplicate symbol the linker happens to pick. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
rx_ancillary_ops_check() silently rewrote a zero-initialized struct st40_rx_ops (type == ST40_TYPE_FRAME_LEVEL by default) to ST40_TYPE_RTP_LEVEL whenever notify_frame_ready was unset and framebuff_cnt was 0. This masked missing ops->type initialization instead of rejecting it, and made the actual session type implicit. Callers must now set ops->type explicitly; st40_test.cpp and RxTxApp's app_rx_anc_init() are updated accordingly. Adds ut40_ops_check_zero_init_with_rtp_callback() to pin that a zero-initialized frame-level ops struct is now rejected with -EINVAL instead of silently reinterpreted. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
gst_mtl_st40p_rx (pipeline API) is the correct ST2110-40 RX element: gst_mtl_st40_rx used ST40P_RX_FLAG_BLOCK_GET, which reintroduces a preroll-timeout bug the pipeline-API plugin deliberately avoids with non-blocking polling, and its include-metadata-in-buffer path truncated multi-bit RFC8331 header fields (line_number, hori_offset, the packed c/s/stream_num byte) into single bytes. It has no capability gst_mtl_st40p_rx lacks, so it is removed rather than kept as a second, worse option. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
Adds a `unit` mode to build.sh that configures the root project with -Denable_unit_tests=true in build_unit/, builds it with ninja, and runs tests/unit/UnitTest. Reuses the existing buildtype/enable_asan flags (e.g. `./build.sh debug unit` builds and runs the suite under ASan via LD_PRELOAD), instead of introducing a separate script with its own env vars. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
dma_dev is a gchar[MTL_PORT_MAX_LEN] member, so its address can never be NULL. GCC's -Waddress flagged the check in gst_mtl_common_parse_general_arguments() as always true; keep only the strlen() emptiness check. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
rx_anc_slot_parse_pkt narrows the running udw_buffer_fill sum into the public st40_meta.udw_offset (uint16_t). The value is currently bounded by ST40_MAX_META entries of at most 0xFF wire bytes each, well under UINT16_MAX, but nothing enforced that relationship if ST40_MAX_META were ever raised. Fail the build instead of silently truncating the offset. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
st40_rx_put_framebuff only checked s_impl->type before dereferencing s_impl->impl, with no refcount protection against a concurrent st40_rx_free() destroying the session underneath it. Use MT_HANDLE_GUARD/MT_HANDLE_RELEASE, the pattern already used elsewhere for this handle type, to close the use-after-free race. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
st20_rx_ops, st22_rx_ops, st30_rx_ops, st40_rx_ops and st41_rx_ops (notify_rtp_ready), plus the matching TX ops structs (notify_rtp_done), documented these RTP-level callbacks as Optional. Each lib create() path actually rejects session creation with -EINVAL when the callback is unset in RTP_LEVEL mode, so the callback is mandatory. Also corrects st30_rx_ops which referenced the wrong enum (ST20_TYPE_RTP_LEVEL instead of ST30_TYPE_RTP_LEVEL). Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
st40_rx_create() logged "st_rx_audio_ops_check fail" on failure, a copy-paste leftover from the audio session code path. It actually calls rx_ancillary_ops_check(). Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
d748dd5 to
d3be896
Compare
st20 video and st40 ancillary RX sessions stamped timestamp_first_pkt with mtl_ptp_read_time(), a software TSC-to-PTP read taken whenever the tasklet got around to processing the packet, including rx-burst batching and scheduling jitter. st30 audio already used mt_mbuf_time_stamp(), which prefers the NIC's HW RX-timestamp dynfield and falls back to the same software read automatically when HW offload isn't enabled or supported. Switch st20 and st40 to mt_mbuf_time_stamp() so all three media types report the first packet's arrival time from the same, most accurate clock source available, with no behavior change on interfaces without HW RX timestamp offload. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
Update the agent routing table, MTL Developer (TDD) and MTL System Admin agent descriptions, the mtl-build and mtl-write-test skills, and the unit test README to reference `./build.sh unit` instead of the stale `./build/tests/unit/UnitTest` path and the manual meson/ninja invocation. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
Three st40 unit test files were added with a stale 2025 copyright year; update to 2026. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>
d3be896 to
400fb14
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
the transport session, fixing recurring
seq_lost/frame-dropregressions on redundant streams; pipeline RX becomes a thin
zero-copy consumer.
st40_rfc8331_*codec helpers (payload sizing, headerbyte-swap, encode/decode) and migrate every call site off duplicated,
hand-rolled wire-format math: RX transport, TX transport, gstreamer
plugin, RxTxApp, and integration/unit test helpers.
duplicated formulas (gstreamer plugin, RxTxApp) that undercounted
payload size by 4 bytes at
udw_size % 16 == 12.BAD_PARITYtest hook's manual per-field bit-packingwith a post-encode corrective pass, so TX can finally call the shared
encoder instead of hand-building packets; fixes an unsigned-underflow
bug this surfaced in the MTU-fit bounds check.
stats_guide.mdwording cleanup, and an MTL system-setupinstructions update (check VF binding before running gtest).
logs into tool output — truncated to a log path + line count + tail.
Commits (10)
st40_rfc8331_{payload_bytes,rtp_hdr_bswap,payload_hdr_bswap,encode_packet,decode_packet}helpersst40_rfc8331_decode_packet()for ANC parsingstats_guide.mdrun_gtestst40_rfc8331_encode_packet()(+ underflow fix)Testing
KahawaiTest --gtest_filter=*St40*: 36/36 pass on real VFs.